S3UTILS-217: improve listObjectsByReplicationStatus.js #367
Conversation
- Show details of the errors when they happen - Display the bucket's name - Display the IsLatest field
Hello scality-gdoumergue,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1 #367 +/- ##
=================================================
- Coverage 43.76% 43.73% -0.03%
=================================================
Files 84 84
Lines 5962 5966 +4
Branches 1256 1256
=================================================
Hits 2609 2609
- Misses 3307 3311 +4
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code itself LGTM, nothing to say here.
But, recently I spent some time to make sure we can test this script in the CI, previously there was none. So if you have the time in hand, and can have a look at this file and if you are able to add some tests that would be great. If not, no worries, I'll take note and create a ticket on my end to do so.
Some notes:
Those tests do require a tool we created to simulate a small s3c env called workbench, to install it:
curl -L -o /tmp/workbench.tar.gz https://github.com/scality/workbench/releases/download/v0.8.0/workbench_Linux_x86_64.tar.gz
tar -xzf /tmp/workbench.tar.gz -C /tmp
sudo install /tmp/workbench /usr/local/bin/workbenchRun it inside s3utils repo, like so:
workbench up --env-dir ./workbench/env -dYou'll then be able to run those particular test like so:
yarn jest --verbose --logHeapUsage --projects jest.config.js --coverage --testPathPattern='tests/functional/listObjectsByReplicationStatus.js'Again, only if you have the time in hand, just let me know.
| Key, | ||
| VersionId, | ||
| IsLatest, | ||
| error: err |
There was a problem hiding this comment.
This can help logging error messages when the message field is dynamically computed:
| error: err | |
| error: err.message |
| }, err => { | ||
| if (err) { | ||
| log.error('error processing batch of objects', { | ||
| error: err, |
|
Thanks @tcarmet , I will use workbench to improve my knowledge! |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| }); | ||
|
|
||
| afterEach(async () => { | ||
| afterAll(async () => { |
There was a problem hiding this comment.
I'd like to leave those as afterEach, for further tests not be impacted by each other and really keep them isolated.
|
|
||
| it('should log an error when using credentials without access to the bucket', async () => { | ||
| // Create a test user with no bucket permissions | ||
| const noPermAccount = await createTestAccount(vaultClient); |
There was a problem hiding this comment.
Let me know what you think, but I don't think we have to bother into creating an account here, we can try to reach accountSource.bucketName with a some kind of fake accessKey and secretKey hardcoded string and expect a authorization denied and the script to fail accordingly with the right message.
| expect(authErrorLog.length).toBe(1); | ||
| }); | ||
|
|
||
| it('should list objects by replication status', async () => { |
There was a problem hiding this comment.
Something that can be updated in this test is to ensure the bucket name is defined when listing an object, since this is something you have updated.
This script has been tested in a lab: